chore: civc tests refactor by maramihali · Pull Request #16086 · AztecProtocol/aztec-packages

maramihali · 2025-07-29T19:34:37Z

In this PR:

unify the logic of the two mock circuit producers for CIVC tests
remove logic in CIVC to handle whether we are or are not receiving precomputed vks and mocked vks, accumulate now expects a precomputed verification key
remove redundant tests

In `stdlib_uint` we no longer need logical operations because the only places they were used in, i.e., std/turbo version of sha256, blake2s, blake3s, have been removed. So its best to reduce complexity of the `uint` class and keep it minimal. Removed the following functions from the `uint` class: ```cpp operator^ operator& operator| operator~ operator>> operator<< ror rol logic_operator ```

…ments as input and returns the commitment to the merged table (#15949) We modify the `MergeVerifier` so that it gets the subtable commitments as input and returns the commitment to the merged table. The reason for this change is that given the new structure of `ClientIVC` following [#15704](#15704), we can't access the merged table commitments from inside `complete_hiding_circuit_logic`. This PR is in preparation for [#15829](#15829) --------- Co-authored-by: AztecBot <tech@aztecprotocol.com>

The tar is not supposed to be checked in according to 50a1bd3#r2229436867

maramihali · 2025-07-30T12:13:14Z

@@ -207,27 +208,6 @@ TEST_F(ClientIVCTests, BadProofFailure)
    EXPECT_TRUE(true);
 };


test above exists in the integration tests

the test seems most suited for this module I feel though.

Yeah that’s true - I think the two test files should just be merged into a single one. Could do it in this PR tbf

kashbrti

will check the rest later. but so far other than a couple clarifications it looks good.

kashbrti · 2025-07-30T17:36:15Z

-        honk_vk->set_metadata(proving_key->get_metadata());
-        vinfo("set honk vk metadata");
-    }
+    honk_vk = precomputed_vk;


hmm, so we don't have any cases where the vk is not provided? was this exclusive to our mock vks?

Well the cases where we were computing the vk on the fly or adding metadata to the the vk was purely for testing and benching purposes. We still mock vks for benchmarks but we do so completely in the test utilities.the accumulate function is meant to be used only with precomputed vks in productions and so I just made tests amenable to production behaviour

kashbrti · 2025-07-30T17:41:04Z

@@ -207,27 +208,6 @@ TEST_F(ClientIVCTests, BadProofFailure)
    EXPECT_TRUE(true);
 };


the test seems most suited for this module I feel though.

kashbrti · 2025-07-30T17:45:26Z

    for (size_t idx = 0; idx < NUM_CIRCUITS; ++idx) {
-        auto circuit = circuit_producer.create_next_circuit(ivc);
-        ivc.accumulate(circuit, precomputed_vks[idx]);
+        auto [circuit, vk] = circuit_producer.create_next_circuit_and_vk(ivc, { .log2_num_gates = 5 + idx });


seems like we were adding 2 to log2_num_gates per index but now we're adding one. Am I missing something here?

It doesn’t matter, the point of the test was to keep increasing the number of gates in the circuit, and that way it could be made cleaner

kashbrti · 2025-07-30T17:45:55Z

        for (size_t j = 0; j < num_circuits; ++j) {
-            auto circuit = circuit_producer.create_next_circuit(ivc, log2_num_gates);
-            ivc.accumulate(circuit);
+            auto [circuit, vk] = circuit_producer.create_next_circuit_and_vk(ivc, { .log2_num_gates = 5 });


feel like this value 5 is being used for a lot of tests, should we make it a global?

Hm, it’s just an arbitrary value, would not want the civc tests to wrongly convey that some circuits should be set to log size 5

We can still have a global called EXAMPLE_LOG_NUM_GATES still I guess.

kashbrti · 2025-07-31T09:41:32Z

-        for (auto& commitment : key->get_all()) {
-            commitment = MegaFlavor::Commitment::random_element();
-        }
+        auto key = idx % 2 == 0 ? app_vk : kernel_vk; // alternate between app and kernel vks


hmm, not sure I saw where we are populating the commitments in the refactored code.

so i'm creating one app vk and one kernel vk above and just using those - the point of these mock vks is to speeed up benchmarks (in the sense that these vks will obviously be wrong but instead of spending time precomputing the vks of many circuits we just do two using our mock circuits so civc will not crash running with them)

kashbrti

other than some comments lgtm.

…m/civc-mock-tests-refactor

This reverts commit 59bc177.

Reverts #16086

See [merge-train-readme.md](https://github.com/AztecProtocol/aztec-packages/blob/next/.github/workflows/merge-train-readme.md). BEGIN_COMMIT_OVERRIDE chore: civc tests refactor (#16086) chore: save memory in trace blocks when gate selectors are known to be all zero (#16044) chore: Revert "chore: civc tests refactor" (#16133) fix: revert "chore: save memory in trace blocks when gate selectors are kn… (#16139) feat: specialised ZeroSelector to save memory on selectors known to be zero (#16141) chore: add a `PG_TAIL` proof and queue type (#15986) Revert "chore: add a `PG_TAIL` proof and queue type" (#16152) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: maramihali <mara@aztecprotocol.com> Co-authored-by: Jonathan Hao <jonathan@aztec-labs.com> Co-authored-by: Suyash Bagad <suyash@aztecprotocol.com> Co-authored-by: federicobarbacovi <171914500+federicobarbacovi@users.noreply.github.com> Co-authored-by: Khashayar Barooti <khashayar@aztecprotocol.com> Co-authored-by: Lucas Xia <lucasxia01@gmail.com>

In this PR (redo of #16086): * unify the logic of the two mock circuit producers for CIVC tests * remove logic in CIVC to handle whether we are or are not receiving precomputed vks and mocked vks, accumulate now * expects a precomputed verification key * remove redundant tests * make our Typescript integration tests also provide precomputed vks for both the native and wasm version of CIVC

AztecBot and others added 11 commits July 25, 2025 03:27

[empty] Start merge-train. Choo choo.

7add7c7

Merge branch 'next' into merge-train/barretenberg

042a9bc

Merge branch 'next' into merge-train/barretenberg

a671f86

Merge branch 'next' into merge-train/barretenberg

bfaaad4

Merge branch 'next' into merge-train/barretenberg

43c3219

Merge branch 'next' into merge-train/barretenberg

7f9ee70

fix: delete tar generated by test-vk-havent-changed script (#15988)

069a1c4

The tar is not supposed to be checked in according to 50a1bd3#r2229436867

Merge branch 'next' into merge-train/barretenberg

95c7e7c

wip

812decc

maramihali force-pushed the mm/civc-mock-tests-refactor branch from bec97b3 to 812decc Compare July 29, 2025 19:35

maramihali added 3 commits July 29, 2025 19:59

everything but ivc recursion constraint

d60df0d

wip

23a4f08

all compiles

edd6e27

maramihali commented Jul 30, 2025

View reviewed changes

maramihali changed the title ~~chore: civc test refactor, unify mock circuits producer~~ chore: civc tests refactor Jul 30, 2025

more wip

c299415

maramihali marked this pull request as ready for review July 30, 2025 12:22

maramihali requested a review from kashbrti July 30, 2025 12:23

maramihali self-assigned this Jul 30, 2025

Base automatically changed from merge-train/barretenberg to next July 30, 2025 17:45

maramihali requested review from IlyasRidhuan, fcarreiro and jeanmon as code owners July 30, 2025 17:45

kashbrti reviewed Jul 30, 2025

View reviewed changes

kashbrti reviewed Jul 31, 2025

View reviewed changes

maramihali changed the base branch from next to merge-train/barretenberg July 31, 2025 09:50

maramihali removed request for IlyasRidhuan and fcarreiro July 31, 2025 09:50

maramihali removed the request for review from jeanmon July 31, 2025 09:50

kashbrti approved these changes Jul 31, 2025

View reviewed changes

maramihali added 4 commits July 31, 2025 10:13

Merge remote-tracking branch 'origin/merge-train/barretenberg' into m…

ef44bf2

…m/civc-mock-tests-refactor

some pr review additions

bfc9d55

Merge remote-tracking branch 'origin/merge-train/barretenberg' into m…

c906ae4

…m/civc-mock-tests-refactor

remove diffs from bad merge

0f3252e

maramihali merged commit 59bc177 into merge-train/barretenberg Jul 31, 2025
4 checks passed

maramihali deleted the mm/civc-mock-tests-refactor branch July 31, 2025 11:33

AztecBot mentioned this pull request Jul 31, 2025

feat: merge-train/barretenberg #16119

Merged

maramihali pushed a commit that referenced this pull request Jul 31, 2025

Revert "chore: civc tests refactor (#16086)"

9652389

This reverts commit 59bc177.

maramihali mentioned this pull request Jul 31, 2025

chore: Revert "chore: civc tests refactor" #16133

Merged

ludamad pushed a commit that referenced this pull request Jul 31, 2025

chore: Revert "chore: civc tests refactor" (#16133)

878f861

Reverts #16086

maramihali restored the mm/civc-mock-tests-refactor branch July 31, 2025 14:46

maramihali mentioned this pull request Aug 1, 2025

chore: civc tests refactor #16159

Merged

		@@ -207,27 +208,6 @@ TEST_F(ClientIVCTests, BadProofFailure)
		EXPECT_TRUE(true);
		};

Conversation

maramihali commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maramihali Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kashbrti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kashbrti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

maramihali commented Jul 29, 2025 •

edited

Loading

maramihali Jul 30, 2025 •

edited

Loading